-
-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Search feature #169
Search feature #169
Conversation
Hi @wiimmers, thanks so much for taking a run at this feature! It's going to be about a week before I can really sit down and review this PR properly. In the meantime, can you please do two things to get this ready for review?
Thanks again, and I'll get back to you next week once I've had a chance to play with this and review it. |
Having issues with the keyutils test, currently. When run traditionally, I am seeing correct results. However, in the test returns an empty result. When testing with an already created credential instead of the generated one, the result is still empty. Tried a few different methods to try and diagnose the issue. I believe it was working when I initially submitted the PR, but could be wrong. Will try to do some research in the coming days but my schedule will be pretty busy until next week as well. |
Are you seeing the test issue on more than one platform? Since each platform requires its own setup (Windows you have to be physically on the machine, Linux headless you have to run the special shell script) I often find that platform-specific test errors are actually platform errors. Also: I notice that the CI tests are failing on one of the original tests on Windows. Perhaps something changed in that code or one of the dependencies? |
@wiimmers Apparently Rust 1.78 introduced a change in the way slices are made from raw bytes. This is the underlying cause of #170. I am fixing this and re-releasing, so you will need to rebase your changes on top of the new master branch. Also this may have been the source of the test errors you were seeing? |
Returns string instead of using print statement. Allows for easier listing of all credentials in the future or custom search by user.
import formatting
Regex for Windows platform only
Tests pass now, although it's just a bandaid for Keyutils. Built the search manually instead of using let result = Search {
inner: Box::new(super::KeyutilsCredentialSearch {}),
}
.by("session", &query); |
Hi. Any updates on this PR? |
I have been trying to get to this. I am hoping to do the review this week. |
Hi @wiimmers! Thanks again for all this work! I have now been able to carefully read all your code. I haven't done any testing yet, because there is a very high-level issue I'd like to clarify with you before going any further. This PR currently looks more to me like the basis of a separate crate rather than an extension of the current keyring crate, for these reasons (none of which are criticisms, mind you):
Given the above, my first thought is that a terrific contribution to the "keyring community of users" would be for you to create a separate crate - perhaps called something like My second thought is that, if you really want search to be a part of this crate, that you restrict the search capabilities to use the generic credential parameters (target, service, and user, all optional), and that you restrict the results to be those that are credentials accessible via the credential API. That would allow you to greatly simplify your search API by having it return credentials (created for the results) rather than bi-level hash maps, and it would allow the search API to be part of the platform-indepenent pluggable keystore API that all the built-in implementations conform to (and thus usable in a platform-independent way). I want to reiterate that none of these thoughts imply any criticism of this contribution. I think this code is of great value to the keyring user community and has been eagerly anticipated by many. Regardless of whether you go the direction of separate crate or restricted search - or perhaps both! - I am very grateful for all this hard work. |
Hey @brotskydotcom, thanks for the feedback! I am ecstatic to have been able to help with this project. I will start getting to work on some next steps sometime this week. I will keep you updated on what I eventually do - but I like the idea of a separate crate AND some restricted searching for this crate. Initially, I had a blank canvas and a broad vision for this feature, so your feedback is valuable moving forward. With this in mind for I appreciate the kind words and would like to say I have had a blast diving into this over the past few weeks. My initial thought is I would like to make this a separate crate first, then move to provide something more in line with what this crate is trying to accomplish. The restrictions made attempting to make this fit |
Hi @wiimmers glad to hear this has been fun for you! I think the idea of doing a separate crate first and then narrowing the functionality to be keyring specific is wonderful. I will be happy to help by contributing search to the mock keystore if/when you get to that point. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See overall review discussion in PR. Let me know if you want me to leave this PR open while you figure out next steps. I look forward to seeing what comes next.
I have created a new repo named keyring-search if anyone wants to keep up with progress. My work machine is Windows so I'm only sure of this platform working. Will have to properly write tests and do a little more digging for FreeBSD and OpenBSD support with the mock module but should be relatively painless to get what is needed for a broad search feature in a separate library. @brotskydotcom if you'd like to keep this PR open for the time being, I don't think that it should take long to get a separate library off the ground before I can take a look at this. A few tweaks and some help with the mock keystore should allow for this to feature to come to fruition! |
I am going to close the PR now that @wiimmers has released this code as a separate crate. |
Basic searching implementation for all platforms and listing the results as a string.
Windows platform has no search functionality built into the windows_sys crate, it uses a custom regex search after getting all credentials.
Secret Service, Keyutils, and System Framework have built in search features that were used and implemented into the basic search structure
Search
. Some more work could be done to get all credentials in the platform specific store so users could perform their own custom search on these stores with custom regex, as well as some functionality to take a search result and create anEntry
from the result for easy password updating.Search::new() initializes the default platform's search structure, from here the
by
method can be called to then take aby
argument and aquery
argument. Each platform'sby
parameter specifies what is and isn't allowed.by
parameter can take any string slice as the attribute keys can differ by app.List and Limit are used to create a string of the credentials formatted from the search results.
A new error type
SearchError(String)
was created to handle errors for this functionality.let result = Search::new()
.expect("Failed to create new search")
.by("session", "keyring-rs:test-user@test-service");
let list = List::list_credentials(result, Limit::All)
.expect("Failed to parse search results to string);